Skip to content

Conversation

@titusfortner
Copy link
Member

@titusfortner titusfortner commented Dec 30, 2025

User description

🔗 Related Issues

.NET implementation of #16809

💥 What does this PR do?

  • ci.yml kicks off on every commit. It runs the new and improved check-bazel-targets.sh
  • Instead of just checking to see if there are any dotnet targets and running the whole ci-dotnet workflow, the unique set of applicable test targets are passed from the script to ci-dotnet.yml and browser tests are only run on those

To get all the tests passing I did:

  • Changed InstallCommand extension handling to support Windows better
  • Truncated the logs to make results easier to read (100 char to match Java), I didn't make it configurable, but we could
  • Submit() to use GetDomAttribute instead of GetAttribute (since we know it is an attribute)
  • A couple test fixes and guards

🔧 Implementation Notes

  • Current implementation is only 2 smoke tests on Windows, this is going to run all the tests for 3 browsers right now; I want to see what is passing right now before restricting it more.

💡 Additional Considerations

  • Total execution time for all of this is: 3h 38m 59s
  • We need define "smoke tests" for these and limit tests
  • We should also create Unit Tests
  • Hmm, and should try to run Safari just to make sure it works.

PR Type

Enhancement


Description

  • Add workflow inputs to ci-dotnet.yml for selective test execution

  • Implement target filtering logic to run only affected .NET tests

  • Expand browser test matrix to cover Chrome, Firefox, and Edge

  • Pass filtered targets from ci.yml to ci-dotnet.yml workflow


Diagram Walkthrough

flowchart LR
  A["ci.yml check job"] -->|"targets output"| B["ci-dotnet.yml"]
  B -->|"filter-targets job"| C["Filter .NET targets"]
  C -->|"filtered targets"| D["browser-tests matrix"]
  D -->|"Chrome/Firefox/Edge"| E["Run bazel tests"]
Loading

File Walkthrough

Relevant files
Enhancement
ci-dotnet.yml
Add selective test execution and browser matrix                   

.github/workflows/ci-dotnet.yml

  • Added workflow inputs targets and run-full-suite to support selective
    test execution
  • Introduced filter-targets job to filter applicable .NET targets from
    upstream workflow
  • Replaced hardcoded integration-tests job with parameterized
    browser-tests job using matrix strategy
  • Added browser matrix for Chrome, Firefox, and Edge with enhanced bazel
    test flags
+54/-6   
ci.yml
Pass filtered targets to .NET workflow                                     

.github/workflows/ci.yml

  • Pass targets output from check job to ci-dotnet.yml workflow
  • Add run-full-suite input to control full test suite execution based on
    event type
  • Removed commit message check from conditional, relying on PR title
    check instead
+9/-1     

@selenium-ci selenium-ci added the B-build Includes scripting, bazel and CI integrations label Dec 30, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 30, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #1234
🟡
🎫 #5678
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Unvalidated targets input: The workflow passes needs.filter-targets.outputs.targets directly into a shell-executed
bazel test command, so a human should verify the upstream targets output cannot be
influenced to inject unexpected shell tokens/flags.

Referred Code
run: >
  bazel test
  --keep_going
  --build_tests_only
  --flaky_test_attempts 3
  --local_test_jobs 1
  --test_tag_filters=${{ matrix.browser }}
  --pin_browsers=false
  --test_env=SE_FORCE_BROWSER_DOWNLOAD=true
  --test_env=SE_SKIP_DRIVER_IN_PATH=true
  ${{ needs.filter-targets.outputs.targets }}

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Dec 30, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify workflow input logic

Simplify the workflow invocation by removing the run-full-suite input and
instead passing an empty targets string to trigger a full suite run, delegating
the logic to the called workflow.

.github/workflows/ci.yml [57-63]

-run-full-suite: >-
+targets: >-
   ${{
-    github.event_name == 'schedule' ||
-    github.event_name == 'workflow_dispatch' ||
-    github.event_name == 'workflow_call' ||
-    contains(github.event.pull_request.title, '[dotnet]')
+    ( github.event_name == 'schedule' ||
+      github.event_name == 'workflow_dispatch' ||
+      github.event_name == 'workflow_call' ||
+      contains(github.event.pull_request.title, '[dotnet]')
+    ) && '' || needs.check.outputs.targets
   }}
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a good refactoring suggestion that simplifies the interface between workflows by using a single targets input, making the logic cleaner and more maintainable.

Medium
Always run build job for caching

Modify the build job to always run, building the specific targets from
filter-targets to improve caching and speed up the workflow.

.github/workflows/ci-dotnet.yml [20-27]

 build:
   name: Build
-  if: ${{ inputs.run-full-suite }}
+  needs: filter-targets
   uses: ./.github/workflows/bazel.yml
   with:
     name: Build
     os: windows
-    run: bazel build //dotnet:all
+    run: bazel build ${{ needs.filter-targets.outputs.targets }}
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a performance inefficiency and proposes a valid optimization to always build the required targets, which improves caching and reduces the runtime of the browser-tests job.

Low
Learned
best practice
Validate and sanitize workflow inputs

Avoid unquoted word-splitting and accept only well-formed Bazel labels to
prevent malformed/empty inputs or unexpected characters from being passed into
the command line.

.github/workflows/ci-dotnet.yml [38-50]

 run: |
   targets="${{ inputs.targets }}"
+  targets="$(echo "$targets" | xargs)"  # trim
   filtered=()
 
-  for t in $targets; do
-    [[ "$t" == //dotnet/* ]] && filtered+=("$t")
-  done
+  if [[ -n "$targets" ]]; then
+    read -r -a arr <<<"$targets"
+    for t in "${arr[@]}"; do
+      [[ "$t" =~ ^//dotnet/[^[:space:]]+$ ]] && filtered+=("$t")
+    done
+  fi
 
   if [ ${#filtered[@]} -eq 0 ]; then
-    echo "targets=//dotnet/..." >> "$GITHUB_OUTPUT"
+    printf 'targets=%s\n' '//dotnet/...' >>"$GITHUB_OUTPUT"
   else
-    echo "targets=${filtered[*]}" >> "$GITHUB_OUTPUT"
+    printf 'targets=%s\n' "${filtered[*]}" >>"$GITHUB_OUTPUT"
   fi
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add explicit validation/guards at integration boundaries (workflow inputs) by trimming and sanitizing before use.

Low
Possible issue
Quote test_tag_filters argument

Add quotes around the ${{ matrix.browser }} variable in the bazel test command
to prevent potential word-splitting issues.

.github/workflows/ci-dotnet.yml [71]

---test_tag_filters=${{ matrix.browser }}
+--test_tag_filters="${{ matrix.browser }}"
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies a potential shell expansion issue and proposes adding quotes to ${{ matrix.browser }}, which improves the robustness of the script.

Low
  • Update

@titusfortner
Copy link
Member Author

@nvborisenko you were right, I didn't need that code. I must have fixed the issue with a different setting.

@titusfortner titusfortner marked this pull request as draft January 9, 2026 23:18
@titusfortner titusfortner force-pushed the dotnet_gha branch 2 times, most recently from c43f4da to 2be7b05 Compare January 10, 2026 14:52
return [browser] + COMMON_TAGS + _BROWSERS[browser]["tags"]

_NUNIT_ARGS = [
"--workers=1", # Bazel tests share a single driver instance; prevent NUnit parallelism
Copy link
Member

@nvborisenko nvborisenko Jan 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, can we temporary remove this arg? Classic tests cannot be executed in parallel, but BiDi tests are designed to be parallelizable. AFAIK NUnit doesn't utilize parallelization if not requested explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's 4 by default. Let me see if we can set it so it only applies to RBE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-build Includes scripting, bazel and CI integrations Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants